Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyring 417 #424

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Keyring 417 #424

wants to merge 22 commits into from

Conversation

spgarbet
Copy link
Member

@spgarbet spgarbet commented Dec 4, 2024

This is the big cut over to using shelter instead of the internal unlockREDCap. It will break local existing keyring stores as it moves to the new format.

This needs careful review before pushing to the main branch.

@couthcommander
Copy link
Contributor

We should have a bridge function that seamlessly transitions a user from "keypass" to "shelter". It would require "keypass" as a suggested package until deprecation. Mark the code with a deprecation date.

@spgarbet
Copy link
Member Author

I really like Cole's idea and will work on implementing.

if(is.null(envir)) dest else list2env(dest, envir=envir)
###########################################################################
## Do it
unlockKeys(connections,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use shelter::unlockKeys

@@ -278,9 +131,8 @@ connectAndCheck <- function(key, url, ...)
#' which returns the keys as a list. Use [globalenv()] to assign in the
#' global environment. Will accept a number such a '1' for global as well.
#' @param keyring character. Potential keyring, not used by default.
Copy link
Contributor

@couthcommander couthcommander Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "keyring" is required - fix may be needed in "shelter::unlockKeys" documentation too

@@ -278,9 +131,8 @@ connectAndCheck <- function(key, url, ...)
#' which returns the keys as a list. Use [globalenv()] to assign in the
#' global environment. Will accept a number such a '1' for global as well.
#' @param keyring character. Potential keyring, not used by default.
#' @param service character. The name to use in a yaml file for locating keys.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe refer back to the expected structure section where the service is "redcapAPI" (which is the default argument, and should rarely if ever be changed)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note, the section above the YAML structure (lines 94-98, at least in this version) reference "keyring_delete" and "backend_file". This needs to be cleaned up (use shelter::keyring_delete).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants